fix(automation): publish $error on the throw arm before deciding whether the failure routes - #16302

Fixes #14955

Step 1 answered: there is no reason, so the asymmetry is the defect

The card set the criterion itself — if there is a reason, it is not recorded at the site and should be; if there is not, the asymmetry is the defect. Three independent readings say there is not:

1. Archaeology. Both arms were born in one commit, cc8484224 ("enhance automation engine with fault edges, step logging, timeout, ..."), and the asymmetry was there at birth. The returned-failure arm was written with its own comment, // Write error output to variable context for downstream nodes, placed above the fault-edge lookup. The throw arm has only // Check for fault edges, with the write inside it. Two adjacent blocks in one bulk commit, written inconsistently — not a decision.
2. Nothing affirmed it since. Every later commit through this region (Node-level error branches: let a flow handle a failed node instead of aborting the run #3863's guard-refusal class, [finding] A dying loop discards its body's completed steps wholesale — the run summary reports acted: 0 while 5 writes actually happened (childSteps splice only after a SUCCESSFUL node result) #13803 / try_catch with no catch region discards its try-region step record on the returned-failure path (the other half of the engine's childSteps asymmetry) #14184's step folds, automation: create_record collapses the engine's DUPLICATE_RECORD envelope to a string, so a flow's try_catch / fault edge still cannot tell "already there" from "the store is down" #14419's code) states in its own comment that it leaves $error semantics untouched. No ADR, no doc and no test records a rationale for the conditional; no test pins the absence of the write.
3. The published contract already says the opposite. content/docs/automation/flows.mdx documents {$error} as naming "the most recent failure only". The conditional violated the contract that was already written down.

So this is the "asymmetry is the defect" branch of the card, and the fix is the one the claim comment named: make the throw arm's refresh symmetric with the returned-failure arm's. The throw shape is untouched — execErr is rethrown exactly as caught, no exported symbol, signature or payload key moves. The wider alternative the card records (attach code to the thrown Error, have try_catch read it off the caught exception) is not taken here.

What changed

executeNode's throw arm now publishes $error and the node-scoped NODE_ID.error entry before deciding whether the failure routes, exactly as the returned-failure arm does. Publishing is not routing: the #3863 guard-refusal rule below the write is untouched and still decides, alone, which failures a fault edge may carry.

A thrown failure publishes { nodeId, message } and nothing more. There is no NodeExecutionResult on that path, so no output and no classified code exist to carry — and that absence is the correct answer for a throw, rather than a reason to leave a stale code standing.

The rationale is now recorded at the site, which was the other half of what the card asked for either way.

Why it mattered, in the card's own words

The message and the code come from two different failures.

A node inside a structured region never has a fault edge of its own — the region's synthetic sub-flow carries only the region's own edges (runRegion). So every thrown failure inside a try_catch, a loop body or any other region left $error naming an earlier, unrelated failure. The failure mode is not a crash but a plausible-looking wrong value: { code: 'DUPLICATE_RECORD', message: "Node 'mk' timed out after 20ms" }, swallowed by a catch region reading it as "the row is already there", with the run reporting success. That is exactly why it stayed invisible, and why the pins below drive flows to different edges rather than asserting a variable.

Pins — three flows, driven end to end

packages/services/service-automation/src/throw-arm-error-refresh.test.ts:

1. A thrown failure with no fault edge of its own names itself on the run-wide $error. An earlier fault-routed create_record duplicate seeds a stale $error; a try_catch binding to $caught (deliberately not $error) then lets its catch region read the engine's own run-wide variable after a node inside the try region throws. Red before, green after.
2. A swallowed failure's code does not leak onto a later thrown failure in the same try region. This is the residual the PR fix(automation): create_record surfaces the engine's DUPLICATE_RECORD code #14948 identity guard cannot see: that guard compares $error against what the attempt started with, so a rewrite happening inside the attempt window — here an inner try_catch binding its own swallowed duplicate — passes the identity check, and the outer container bound DUPLICATE_RECORD onto a timeout's message and swallowed it. The two outcomes take different edges (reraised then escalate, versus a silent swallow). Red before, green after.
3. A thrown guard refusal is still un-routable. With a stale $error seeded first, so the new unconditional write is actually exercised on that path: the run still aborts with the guard's own message, the handler behind the fault edge never runs, and the destructive verb is never reached. Green before and after — a guard-rail, not a red-first pin, and stated as such.

On PR #14948 — not re-litigated

That PR's minimal identity guard and its two flow pins are left in place and unchanged. Its guard now has a narrower job rather than none: a throw that is not a node failure at all (a durable pause refused inside a region, a missing region entry — anything runRegion raises before reaching a node) still never touches $error, and the stale value must not be claimed then either. Two comments that described the old engine behaviour and became false with this change are corrected in place, without touching a single assertion.

Also unrelated to issue #15646 (same file, undispatched): nothing here touches runRegion or the pause path, and origin/main carried no packages/services/service-automation/ change between this branch's base and its merge of current main.

Verification — all on the final head 577a51e15

• Ablation, direction predicted before running: putting both writes back inside if (faultEdge) should turn pins 1 and 2 red on the same two assertions and leave pin 3 green. Observed exactly that: 2 failed | 1 passed, expected 'a' to be 'boom' and expected 'DUPLICATE_RECORD' to be undefined. The mutation was confirmed on disk before the run (the 16-space spelling went 1 to 0, the 20-space spelling 0 to 1, and git hash-object differed from the HEAD blob), and the restore leg was confirmed the same way (hash identical to the HEAD blob, counts back to 1 and 0, git diff HEAD empty). No rebuild leg is owed: the pins import the subject as ./engine.js, a same-package relative specifier vitest resolves to src, not dist.
• An earlier reading in the other direction, taken before the implementation existed: the same three pins on the unmodified tree gave 2 failed | 1 passed with those same two assertions.
• pnpm --filter @objectstack/service-automation test — 120 files, 1422 tests, all passing.
• pnpm --filter @objectstack/service-automation typecheck — clean, and check:test-typecheck reports the test layer compiling with 0 files / 0 errors in the ledger. Neither tsconfig in this package excludes *.test.ts, so the new pin file is inside both programs.
• Gate family derived mechanically on this head — node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, which read the change set from git itself (5 paths) and printed 56 commands. All 56 run, each exit code captured immediately after a single redirection with no pipe anywhere. 54 were green first time. Two returned exit 3, PREREQUISITE NOT MET, and were converted into real measurements rather than reported as passes: check:dual-build-cjs-loads needed the built closure (green after turbo run build over the packages), and check:type-check-debt needed the heap ceiling lint.yml declares for that step, --max-old-space-size=6144 (green at that ceiling; at 4096 its tsc re-measure died of heap exhaustion, which the gate correctly refuses to record as zero).
• The four artifact-roster families the derivation flags as keeping their roster in a directory one of these paths is in — check-changeset-fixed, check:authz-resolver, check:error-code-casing, check:filter-alias-parity — were run rather than read as silent. All green.
• pnpm lint over the whole repository — green. No narrowing claimed and none needed.

A changeset is included: this changes runtime behaviour a flow author can observe, so it is not a skip-changeset shape.

🤖 Generated with Claude Code

Generated by Claude Code